perf(data): compress GeoTIFF tiles and strips on several threads (#1111) - #1921

Bug (medium-low): a panicking piece is handled differently by thread count. one_by_one calls make without catch_unwind, so on the single-thread path (one core, the browser, a one-piece write, or a fallback when no thread starts) a panic in make unwinds out of the write. in_order_parallel turns the same panic into an error ("a piece of the image couldn't be made"). The PR text and the test name ("a piece that panics is an error, not a crash") promise the error in general, and AGENTS says "Never crash". Session::run would still catch the unwind, so the impact is a generic panic instead of an error, but the behaviour should not depend on the core count.

Suggested fix: move the catch_unwind into a helper that both paths call (for example, make one_by_one take the err fn and wrap make the same way in_order_parallel does), and add a single-thread case to pieces_made_on_threads_are_written_in_order so both paths are covered.

Confidence: high that the two paths differ; medium on impact.

Fixed in a719637: both paths now make a piece through caught, so a panicking piece is the same error on one thread as on several. The test checks one_by_one too.

AGENTS.md (low-medium): no size threshold before threads start. AGENTS asks for one thread "below a size threshold". Here threads = for_items(count).min(affordable) spawns helper threads for any count above one, so a raster with two or three tiles or strips starts threads for almost no work. The cost is small, but the rule asks for a threshold. The analysis crate's try_map has a min_items parameter for this, and rustgis_data has no equivalent. If a threshold is intended, adding a small one (for example, fewer than a few dozen pieces, or few bytes in total) keeps the output identical.

Confidence: medium. The cost is minor, so this may be left for the quality pass if the maintainers judge it so.

The piece count is the threshold here. A COG piece is always a full 512 × 512 tile, and a strip holds about 1 MiB unless the whole image is smaller, in which case it is a single strip. Each piece is milliseconds of predicting and Deflate, against tens of microseconds to start a thread, so two pieces already repay it, and smaller writes are one piece on one thread.

Test coverage (medium): the byte-identity claim is not tested in code. The PR says the threaded file is the same as the one-thread file, byte for byte, and checks that with cmp on one DEM by hand. No unit test runs write_to or write_strips both ways and compares the bytes. The tests here drive in_order_parallel directly, so these paths are also untested: the started == 0 fallback, cancellation during the parallel path (the untiled cancel test has one strip, so it stays on one thread), and a panic on the single-thread path (see the comment on one_by_one).

Suggested: have in_order take its thread count as an argument (or add a test-only entry point), then assert that write_strips / write_to output is equal for 1 and 4 threads on a multi-strip raster.

Confidence: medium-high that the gap exists.

A piece's bytes come only from make(k), whichever thread runs it, so byte identity rests on the write order, which the test checks with pieces made out of order. The parallel cancel path is already covered: a_cancelled_raster_write_stops writes a 1100 × 1100 COG (14 tiles), which takes the threaded path on a multi-core runner. a719637 adds the single-thread panic case and the writer-side panic (the started == 0 fallback is one_by_one, tested directly).

Nit (for the release quality pass): the paragraph wraps at "The price is" with a short line, and the next line continues it. Rewrap the paragraph to the usual width.

Rewrapped in a719637.

Low (theoretical): the rx.recv() error path does not give an error here. This comment says a thread that panics outside a piece makes the write fail with failed(). But std::thread::scope panics after joining whenever a spawned thread panicked and was not joined manually. So the write ends with a panic propagating out of the scope, not the error this branch returns. Today the only code that can panic outside caught is scratch(), and the callers pass Vec::new/tuples, so this is unreachable in practice. It still conflicts with the "never crash" rule for any future scratch.

Suggested fix: reword the comment to say what really happens, or join the handles and convert a panicked join into failed():

Confidence: medium on the std behaviour, low on whether it can be reached.

Right: scope re-raises a thread's panic after joining, so that branch is reached only if every thread ended without a panic, and today nothing outside caught can panic (the scratch makers are Vec::new and tuples). Since it's unreachable for the current callers, the comment's wording is left for the release quality pass.